Skip to content

Fixes #1684 when installing with yarn online#1685

Closed
lpalmes wants to merge 1 commit into
react:masterfrom
lpalmes:master
Closed

Fixes #1684 when installing with yarn online#1685
lpalmes wants to merge 1 commit into
react:masterfrom
lpalmes:master

Conversation

@lpalmes

@lpalmes lpalmes commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

This was a quick fix of #1684 which i used to create a new project and the "false" : "0.0.4" dependency was gone.

args = [
'add',
'--exact',
isOnline === false && '--offline'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried using a ternary operation here but they all failed, and i didn't want to apply a filter function after concat because i think it obscures the intent.

@Timer

Timer commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

Oops! I resolved this in master already.
Sorry about this, I didn't see this PR and I started working on the bug immediately when I saw the issue. 😨

I really hope to see contributions from you in the future.

@Timer

Timer commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

This is out in create-react-app@1.2.1. Please verify it works. 😄

@Timer Timer closed this Feb 28, 2017
@lpalmes

lpalmes commented Feb 28, 2017

Copy link
Copy Markdown
Contributor Author

That's fine, just wanted to fix this for the sake of the project, i hope i will have some contributions in the future 😄 .
Thanks tim, i'm going to close this then, i guess you can close the issue now that has been fixed 🎉

@lpalmes

lpalmes commented Feb 28, 2017

Copy link
Copy Markdown
Contributor Author

@Timer
Ahhh, much better. Glad this didn't evolve into leftpad 😆

"dependencies": {
    "react": "^15.4.2",
    "react-dom": "^15.4.2"
  },

@Timer

Timer commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

Thanks for catching it! If you'd like, I'd appreciate an added test case which makes sure we don't install any dependencies other than those 3.

@lpalmes

lpalmes commented Feb 28, 2017

Copy link
Copy Markdown
Contributor Author

Sure, i can take a shot at it, should that be in the file, like check for the correct dependencies and if we found other than react, react-dom, react-scripts just process.exit(1)?

@Timer

Timer commented Mar 1, 2017

Copy link
Copy Markdown
Contributor

Yeah you can add it to e2e-installs.sh and either call a node script that inspects the package or use a few unix commands. Unix commands would probably be preferred, but Node totally acceptable.

@Timer

Timer commented Mar 1, 2017

Copy link
Copy Markdown
Contributor

I'm sure you could use awk and accomplish this, or grep depending on the version.

@gaearon

gaearon commented Mar 1, 2017

Copy link
Copy Markdown
Contributor

Thanks for the quick fix. 😅

@lock lock Bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants